Translate _d_arrayappend{T,cTX} to templates by Vild · Pull Request #2632 · dlang/druntime

Vild · 2019-06-06T00:20:48Z

This PR adds templated version of _d_arrayappend{T,cTX}.

Notes

There are wrapper around each hook to fake the safety level of the real implementations to not break code.
I only translated the entrypoints for the hooks and not the called functions. Because this would have taken took long. Future working can be done to rewrite these functions.
_d_arrayappend{c,w}d were not translated because they did not use TypeInfo in their interface. But they were changed to use the new hooks.

thewilsonator · 2019-06-06T03:36:02Z

This uses scope, this should also be using return ref if I am understating the code correctly. This may alleviate the need for trusted.

src/object.d

JinShil · 2019-06-06T06:14:10Z

src/object.d

+// These are needed to fake the safety level to `@safe pure nothrow`.
+// Without this code will break as these hooks where previously exported
+// as having this safety level.
+T[] _d_arrayappendT(T)(ref T[] x, scope T[] y) @trusted


Do we need to return T[]?

Is the @trusted on this declaration redundant? It seems that since the call to _d_arrayappendT_ is wrapped @trusted. the @trusted attribute at entry point is not necessary.

Do we need to return T[]?

For chaining?

It returns T[] because the original hook returned void[]. I also just found a problem with the current implementation in dmd:

int[] arr; ((arr ~= [1,2,3]) ~= 4) ~= 5;

This crashes in the backend for dmd (https://run.dlang.io/is/UZAIuX), while working as expected in ldc (arr == [1, 2, 3, 4, 5]).
So I should probably change this to return a ref T[], to match ldc's behavior.

I think I still need @trusted for the outer functions as I get this error if I remove @trusted on _d_arrayappendcT.

std/uni.d(6165): Error: @safe function std.uni.Stack!(InversionList!(GcPolicy)).Stack.push cannot call @system function object._d_arrayappendcT!(InversionList!(GcPolicy))._d_arrayappendcT

src/object.d

JinShil · 2019-06-06T06:45:23Z

Should I keep the changes to _d_arrayappend{c,w}d for this PR?
If those changes gets removed and this PR and dmds PR gets merged. The old code will still be used when assigning wchar and dchars to char[]. But then yet again, those changed do not really belong in this PR.

~~I'm not sure. Should that have been a prerequisite to this PR?~~

Sorry, I misunderstood the question. I would prefer to keep the changes in this PR so we don't have two different implementations doing the same thing.

jpf91 · 2019-06-06T07:12:14Z

libphobos2.a(process.o):std/process.d:_D3std7process12__ModuleInfoZ: error: undefined reference to '_D3std8internal6memory12__ModuleInfoZ'
I have not figured out what causes this to happen. I guess it is something related to the templates using internal imports, but I'm not sure. I would appreciate some help.

The really strange thing here and the reason this fails to link is that std.internal.memory is in phobos.
Can you run process.o through objdump and see where exactly the symbol is referenced?

_D4core3sys7openbsdQm5cdefs12__ModuleInfoZ

Also strange The core modules are not compiled into the library, so there's not ModuleInfo, TypeInfo, ... emitted for these. There's nothing in cdefs.d which would need ModuleInfo though and I wonder why it even pulls in OpenBSD code?

jacob-carlborg · 2019-06-06T07:28:13Z

Also strange The core modules are not compiled into the library, so there's not ModuleInfo, TypeInfo, ... emitted for these.

I’ve run into this before. The C bindings are not being compiled. So the few macros that have been translated to functions are not compiled and causes linker errors.

jacob-carlborg · 2019-06-06T07:29:48Z

There's nothing in cdefs.d which would need ModuleInfo though and I wonder why it even pulls in OpenBSD code?

Perhaps compiling with -vgc-ast will reveal something.

jacob-carlborg

Can all the new symbols be private and have the compiler bypass the protection?

Vild · 2019-06-06T12:43:01Z

Should I keep the changes to _d_arrayappend{c,w}d for this PR?
If those changes gets removed and this PR and dmds PR gets merged. The old code will still be used when assigning wchar and dchars to char[]. But then yet again, those changed do not really belong in this PR.

I'm not sure. Should that have been a prerequisite to this PR?

I think I will remove that commit, and then apply it later when I do a clean-up PR.

rainers · 2019-06-06T17:45:44Z

src/object.d

+
+T[] _d_arrayappendcTXTrace(T)(string file, int line, string funcname, ref T[] px, size_t n) @trusted pure
+{
+    import rt.profilegc : accumulate;


Note that the implementation of the tracing functions has changed recently, see #2607

I wouldn't mind if the trace-functions are left unmodified, it's a half-baked feature anyway and slows down execution in accumulate.

The problem is that the new entrypoints for the hooks are templates, so I need to manually write a trace entrypoint. It also need to be in object.d so the compiler can reference it from the lowering code. https://github.com/dlang/dmd/pull/9982/files#diff-0c1aa1ff99402eb87e11cc3fa1b80da0

Vild · 2019-06-11T01:16:32Z

I've updated the code according to the comments, but the unittest will still fail. This time because when I changed the compiler to rewrite arr ~= elem to _d_arrayappendcT(arr, elem) it will call the postblit twice on elem, once before the hook is called, and once during the hook. Here I either need to figure out how to never make it call the postblit before calling the function, or revert to the previous behavior where the assignment step is done outside the hook.

I intent on fixing this as soon as possible.

Vild · 2019-06-11T23:50:44Z

Update:
All the druntime unittests passes.
In phobos everything related to CodepointSet fails, like this one: https://github.com/dlang/phobos/blob/master/std/uni.d#L6916

posix.mak

JinShil · 2019-06-22T23:16:10Z

src/object.d

+/// See $(REF capacity, rt,array,capacity)
+public import rt.array.capacity: capacity;
+
+/// See $(REF _d_arrayappendT, rt,array,append) and $(REF _d_arrayappendTTrace, rt,array,append)


You actually don't need to document these, because they are not called directly by the user. capacity.d has functions that are called directly by the user, so it is an exception. I'd like to add a version(Core_Doc) block to filter out the runtime hooks so they don't appear in the documentation. I wouldn't mind if you added that version block to this PR, but I understand if you want to keep the scope of this PR down. I intend to add it myself after my pending druntime PRs are merged.

Okey, thanks.

It think it is better to do the version(Core_Doc) in a separate PR.

src/object.d

dlang-bot · 2019-06-23T00:50:43Z

Thanks for your pull request and interest in making D better, @Vild! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2632"

JinShil · 2019-06-23T01:19:16Z

src/rt/array/append.d

+// release. It also cannot be core.internal.externDFunc'd because that causes recursive template instantiation.
+
+/// See $(REF _d_arrayappendcTX, rt,lifetime,_d_arrayappendcTX)
+private extern (C) byte[] _d_arrayappendcTX(const TypeInfo ti, ref byte[] px, size_t n);


I think I commented about this before, but I can't find my comment now. I think this can be imported instead of being declared as an extern

I don't think it can because it will be instantiated inside the template and rt.lifetime not part of a dmd installation.

Ok, it looks like we'll eventually need to change the DMD installation to make all this templating of druntime work, but that is out of scope of the task at hand.

src/rt/array/append.d

JinShil · 2019-06-23T08:56:11Z

src/rt/array/append.d

+        // This template will be instanciated even in betterC due to the lowering code.
+        // So we need to disallow the calling of this function if that happens.
+        version (D_BetterC)
+            assert(0, "_d_arrayappendcTX does not support betterC!");


What happens if you don't specifically have this check. Won't the -betterC build just emit a generic error about not supporting TypeInfo?

Also, if you do choose to keep this message, I don't think it should specifically mention _d_arrayappendcTX because that never appears in the user's code. I suggest something worded more as "betterC does not support appending arrays because it requires runtime support".

And I think that it can be a static assert at the beginning of the _d_arrayappendcTX template so the user gets a compile-time assertion failure instead of a runtime assertion failure.

So without this check test19561.d will fail with Error: TypeInfo cannot be used with -betterC, even if that expression will be intercepted by the CTFE later.

and it cannot be a static assert because that breaks the lowering code.

import/rt/array/append.d(121): Error: static assert: "_d_arrayappendcTX does not support betterC!" import/rt/array/append.d(35): instantiated from here: _d_arrayappendcTX!(immutable(char)) import/core/internal/arrayop.d(320): instantiated from here: _d_arrayappendT!(immutable(char)) import/core/internal/arrayop.d(45): instantiated from here: initScalarVecs!(int, "+=") import/object.d(3696): instantiated from here: arrayOp!(int[], int, "+=")

src/rt/array/append.d

jpf91 · 2019-06-30T10:26:15Z

So as it seems we can't avoid the D_BetterC workaround right now, after changing the version condition to D_TypeInfo, is there anything left to be done here or is this ready?

Code LGTM.

Vild · 2019-06-30T19:42:47Z

[...] is there anything left to be done here or is this ready?

I did the last few fixes and I'm happy with it for now.
In the future I would like to add nothrow deducting, but as it still goes though the old hooks it is pretty hard to add right now.

Vild · 2019-07-08T11:08:43Z

This PR needs to implements the same fixes as in #2667, before it is ready to be merged

RazvanN7 · 2019-07-15T08:29:22Z

@Vild Can you rebase this to get rid of the conflicts and see if we can get this in?

Vild · 2019-07-15T10:42:42Z

@Vild Can you rebase this to get rid of the conflicts and see if we can get this in?

This PR requires #2673.
So as soon as I fix that one and it gets merge I will rebase this one.

thewilsonator · 2019-07-15T13:16:43Z

and it gets merge

Done.

…itNoThrow` Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Vild · 2019-07-21T16:29:46Z

This PR is ready to be merged

Vild mentioned this pull request Jun 6, 2019

Translate _d_arrayappend{T,cTX} to templates dlang/dmd#9982

Closed

JinShil reviewed Jun 6, 2019

View reviewed changes

src/object.d Outdated Show resolved Hide resolved

JinShil reviewed Jun 6, 2019

View reviewed changes

src/object.d Outdated Show resolved Hide resolved

JinShil reviewed Jun 6, 2019

View reviewed changes

src/object.d Outdated Show resolved Hide resolved

JinShil reviewed Jun 6, 2019

View reviewed changes

src/object.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Jun 6, 2019

View reviewed changes

rainers reviewed Jun 6, 2019

View reviewed changes

Vild force-pushed the ConvertArrayAppendHook branch from cec2e74 to b0ea4b9 Compare June 11, 2019 01:11

Vild force-pushed the ConvertArrayAppendHook branch 3 times, most recently from 3581270 to b572d12 Compare June 11, 2019 18:35

Vild force-pushed the ConvertArrayAppendHook branch from 1d7120f to 1592adb Compare June 22, 2019 22:08

Vild changed the title ~~[WIP] Translate _d_arrayappend{,c}T to templates~~ [WIP] Translate _d_arrayappend{c,cTX} to templates Jun 22, 2019

Vild changed the title ~~[WIP] Translate _d_arrayappend{c,cTX} to templates~~ [WIP] Translate _d_arrayappend{T,cTX} to templates Jun 22, 2019

Vild force-pushed the ConvertArrayAppendHook branch from 1592adb to e1c1e2b Compare June 22, 2019 22:26

JinShil reviewed Jun 22, 2019

View reviewed changes

posix.mak Outdated Show resolved Hide resolved

JinShil reviewed Jun 22, 2019

View reviewed changes

Vild force-pushed the ConvertArrayAppendHook branch 2 times, most recently from 10bb660 to 530bd7e Compare June 22, 2019 23:45

JinShil reviewed Jun 23, 2019

View reviewed changes

src/object.d Outdated Show resolved Hide resolved

Vild force-pushed the ConvertArrayAppendHook branch from 530bd7e to 238a29a Compare June 23, 2019 00:50

Vild requested review from Burgos, MartinNowak, schveiguy and wilzbach as code owners June 23, 2019 00:50

Vild changed the title ~~[WIP] Translate _d_arrayappend{T,cTX} to templates~~ Translate _d_arrayappend{T,cTX} to templates Jun 23, 2019

JinShil reviewed Jun 23, 2019

View reviewed changes

src/rt/array/append.d Outdated Show resolved Hide resolved

JinShil reviewed Jun 23, 2019

View reviewed changes

Vild force-pushed the ConvertArrayAppendHook branch from 238a29a to 56c4ad0 Compare June 23, 2019 21:22

jpf91 reviewed Jun 30, 2019

View reviewed changes

src/rt/array/append.d Outdated Show resolved Hide resolved

Vild force-pushed the ConvertArrayAppendHook branch from 56c4ad0 to 595c17f Compare June 30, 2019 19:37

Vild force-pushed the ConvertArrayAppendHook branch from 595c17f to ddb55e7 Compare June 30, 2019 21:10

Vild added 3 commits July 19, 2019 14:03

Remove nothrow from _d_HookTraceImpl, Add isNoThrow and `isPostbl…

2adb0eb

…itNoThrow` Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Add templated _d_arrayappend{c,cTX} with tests

155c8f8

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Use object._d_arrayappendTImpl in _d_arrayappend{c,w}d

cd53555

Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>

Vild force-pushed the ConvertArrayAppendHook branch from ddb55e7 to cd53555 Compare July 19, 2019 12:10

thewilsonator approved these changes Jul 21, 2019

View reviewed changes

thewilsonator added the auto-merge-squash label Jul 21, 2019

dlang-bot merged commit 227bcbf into dlang:master Jul 21, 2019

JinShil mentioned this pull request Aug 1, 2019

Make core.internal.convert usable in -betterC #2710

Closed

teodutu mentioned this pull request May 26, 2022

Remove previously templated hooks #3826

Merged

dkorpel mentioned this pull request Jul 4, 2022

Remove string mixin from _d_arrayappendcTX #3870

Merged

Uh oh!

Conversation

Vild commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Uh oh!

thewilsonator commented Jun 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JinShil commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpf91 commented Jun 6, 2019

Uh oh!

jacob-carlborg commented Jun 6, 2019

Uh oh!

jacob-carlborg commented Jun 6, 2019

Uh oh!

jacob-carlborg left a comment

Choose a reason for hiding this comment

Uh oh!

Vild commented Jun 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vild commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vild commented Jun 11, 2019

Uh oh!

Uh oh!

JinShil Jun 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dlang-bot commented Jun 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil Jun 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpf91 commented Jun 30, 2019

Uh oh!

Vild commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vild commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vild commented Jun 6, 2019 •

edited

Loading

JinShil commented Jun 6, 2019 •

edited

Loading

Vild commented Jun 11, 2019 •

edited

Loading

JinShil Jun 22, 2019 •

edited

Loading

dlang-bot commented Jun 23, 2019 •

edited

Loading

JinShil Jun 23, 2019 •

edited

Loading

Vild commented Jun 30, 2019 •

edited

Loading

Vild commented Jul 8, 2019 •

edited

Loading